-
Notifications
You must be signed in to change notification settings - Fork 865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Utilize all insert results of the new recv buffer. #2535
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Would you be able to make it more "friendly" against changes in #2527? |
Did you mean to introduce |
I'm not exactly convinced about adding this parameter so that it can be better displayed in logs. As for this, might be a better idea to instead move the whole log into the receiver buffer internals. In that PR the discrepancy is also detected on the level of insert() call, and it is also reported to the caller - not that the caller recognizes this by checking the range of the returned value. |
I'm OK for this, but I don't want to change too much codes at once to keep a single PR small. |
I understand, but if you provide changes that will make them harder to merge with my PR, it will be counter-productive. If this value is only used in logs, it should be able to be taken as before. Important would be if this value is to be used for some checks involved in the overall functionality. |
You might consider borrowing the definition of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make sense to declare an enum
of the return values of the CRcvBuffer::insert(..)
function.
enum {
RBUF_INSERT_RES_OK = 0,
RBUF_INSERT_RES_DUP = -1, // Duplicated (redundant).
RBUF_INSERT_RES_BALETED = -2,
RBUF_INSERT_RES_NO_ROOM = -3,
};
In #2527 there's something kinda better. There is an enum and localized to the buffer class. |
So to reduce conflicts, I would not do this in this PR. I will address other comments tomorrow. |
@gou4shi1 Thank you for the PR anyway! These changes are to come one day. :) |
As https://github.com/Haivision/srt/pull/2530/files#r1020280948 said.
This PR decouples
handleSocketPacketReception()
fromgetAvailRcvBufferSizeNoLock()
.The "no room" condition now only relies on the recv buffer itself.